Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(update): add json-merge-patch guidance #206

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

toumorokoshi
Copy link
Member

After offline discussion, it was agreed upon that IETF RFC 7396 (JSON Merge Patch) is the more sensible choice for a partial update on an HTTP+JSON API. The rationale includes:

  • can generally be expressed via protobuf field masks.
  • aligns with the AEPs usage of resource-oriented guidance.
  • aligns with an existing IETF standard, which as already been widely adopted.

Other minor changes:

  • removes duplicative guidance around LROs to make the AEP succinct.
  • populates oas guidance, sans etags which may require some more design (in HTTP etags are not added to the resource body).

Fixes #109.

🍱 Types of changes

What types of changes does your code introduce to AEP? Put an x in the boxes
that apply

  • Enhancement
  • New proposal
  • Migrated from google.aip.dev
  • Chore / Quick Fix

📋 Your checklist for this pull request

Please review the AEP Style and Guidance for
contributing to this repository.

General

Additional checklist for a new AEP

💝 Thank you!

@toumorokoshi toumorokoshi requested a review from a team as a code owner August 9, 2024 23:09
@toumorokoshi
Copy link
Member Author

FYI @dv-stewarts if you'd like to take a look and give your thoughts.

aep/general/0134/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0134/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0134/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0134/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0134/aep.md.j2 Outdated Show resolved Hide resolved
@dv-stewarts
Copy link
Contributor

AEP includes guidance on HTTP and gRPC transcoding. Will supporting update_mask for gRPC and json-merge-patch for OAS cause the transcoded gRPC APIs to be invalid wrt the OAS spec? Is that a common occurrence and is it viewed as a problem?

We have been using gRPC transcoding as a way to get people into gRPC on the server side without having to fight so many battles with API consumers. Having "native" OAS APIs working differently than transcoded APIs while both claiming to be AEP compliant is likely to sow confusion.

@toumorokoshi
Copy link
Member Author

@dv-stewarts Thanks for taking a look!

AEP includes guidance on HTTP and gRPC transcoding. Will supporting update_mask for gRPC and json-merge-patch for OAS cause the transcoded gRPC APIs to be invalid wrt the OAS spec? Is that a common occurrence and is it viewed as a problem?

We have been using gRPC transcoding as a way to get people into gRPC on the server side without having to fight so many battles with API consumers. Having "native" OAS APIs working differently than transcoded APIs while both claiming to be AEP compliant is likely to sow confusion.

It isn't a very common occurrence - and ideally we wouldn't deviate. There are a lot of tensions w.r.t. this problem overall, including:

  • different features in protocols (e.g. protobuf 2 will default non-nullable fields, while in json all fields are nullable).
  • HTTP+JSON already has a rich ecosystem of IETF RFCs, many with significant adoption and also not looking like AEP guidance with existing protobufs.

I don't think there's a lot of conflicts generally between the protobuf and HTTP+JSON APIs in the context of the AEPs. and when there are, we try to reconcile them. I don't think we've agreed 100% on how this should be addressed. The conclusion we have reached so far is:

  • json-merge-patch is probably the best solution so far (sans yet another standard) for PATCH in JSON.
  • field masks are not a great solution for protobuf, but is already widely adopted.

Ideally, I think we'd replace existing protobuf guidance with something that aligns better with json-merge-patch. However, json-merge-patch uses the resource as the schema, while in protobuf that's not really an option given that the message schema may include non-nullable fields, which mean that you can't really send the intent that "you don't care about the field" without some sort of sidecar (in this case, the field mask).

At Google IIUC json-merge-patch is supported (e.g. https://cloud.google.com/workflows/docs/reference/googleapis/compute/v1/backendBuckets/patch), and there is a transcoding layer that handles the conversion of the patch to the protobuf field mask.

If you have a specific suggestion, it's definitely up for discussion! I'd encourage you to share any ideas you may have.

@toumorokoshi toumorokoshi requested a review from mkistler August 23, 2024 19:13
@toumorokoshi
Copy link
Member Author

@mkistler adding you as an official reviewer, thank you!

Copy link
Contributor

@mkistler mkistler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one suggestion for a minor wording change.

I'm also unclear if we should allow "application/json" content type.

Otherwise this looks good.

aep/general/0134/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0134/aep.md.j2 Outdated Show resolved Hide resolved
aep/general/0134/aep.md.j2 Outdated Show resolved Hide resolved
@fsaintjacques
Copy link

I find it confusing that this would be a JSON first solution where-as this is a protobuf driven spec. I'd much prefer to stick with FieldMask than having to deal with this required infrastructure (transcoding from json-patch to grpc-fieldmask). That almost makes me ignore aep as a potential user.

@toumorokoshi
Copy link
Member Author

I find it confusing that this would be a JSON first solution where-as this is a protobuf driven spec. I'd much prefer to stick with FieldMask than having to deal with this required infrastructure (transcoding from json-patch to grpc-fieldmask). That almost makes me ignore aep as a potential user.

Thanks for the feedback! It's worth clarifying - aep is not a proto-driven spec. It is intended to be protocol agnostic principles, with a precise specification in multiple protocols (grpc and HTTP+JSON are the two in scope today). We're working on better docs, but there's a video where the project goals are explained: https://www.youtube.com/watch?v=we9Qp2kHsvY&t=1s.

That all said I do acknowledge the difficulty in building a proper transcoding layer when the interface is so different. I think we'd like to introduce tooling (possibly building adapter services similar to grpc-gateway, or extend / contribute to it). But in the weekly meeting discussions we've agreed that we should be provide idiomatic guidance for each protcol we support, and diverge only when there's complete incompatibility between the two. In this case, we discussed that it's better to choose the widely supported json-merge-patch specification. (But of course, a separate maintainer has to approve this guidance to ratify it).

If there is an alternate idiomatic interface that works well for both, definitely interested to hear of one!

@toumorokoshi toumorokoshi added this to the GA milestone Nov 5, 2024
@toumorokoshi toumorokoshi mentioned this pull request Nov 13, 2024
11 tasks
@toumorokoshi toumorokoshi force-pushed the yft/json-merge-patch branch 3 times, most recently from 019a786 to 73486cc Compare November 19, 2024 06:49
@toumorokoshi
Copy link
Member Author

I've also added an explanation of json-merge-patch: As an FYI, grpc-HTTP bindings such grpc-gateway support this patch-style translation, so I think ultimately this guidance is actual interoperable with existing proto and proto-http-binding best practices.

@toumorokoshi
Copy link
Member Author

@gibson042 @dv-stewarts @mkistler sorry for the long delay on this one - I've rebased the content, removed the guidance to suggest application/json, and added a blurb explaining why this actually works out ok for both HTTP and proto. PTAL, and I appreciate the dialog!

… Merge Patch) is the more sensible choice for a partial update on an HTTP+JSON API. The rationale includes:

- can generally be expressed via protobuf field masks.
- aligns with the AEPs usage of resource-oriented guidance.
- aligns with an existing IETF standard, which as already been widely adopted.

Other minor changes:

- removes duplicative guidance around LROs to make the AEP succinct.
- populates oas guidance, sans etags which may require some more design (in HTTP etags are not added to the resource body).

Fixes aep-dev#109.
@toumorokoshi toumorokoshi merged commit c70e65a into aep-dev:main Nov 26, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

discuss the best PATCH semantic to update
5 participants